fix(timestamp conversion): use timestamp_nanos_opt to avoid panics
#979
Conversation
pront
changed the title
timestamp_nanos_opt to avoid panics
There was a problem hiding this comment.
I think this is a breaking change, no? I think the fallibility of these functions needs to be updated. We'll also want to document how to migrate.
Nit: I think we could use clippy to disallow calls to timestamp_nanos() to avoid accidentally introducing a panic in the future. See https://rust-lang.github.io/rust-clippy/master/#/disallowed_method
| @@ -0,0 +1 @@ | |||
| Replaced all usages of `timestamp_nanos` with `timestamp_nanos_opt` to avoid panics when the timestamp is out of range. | |||
There was a problem hiding this comment.
| Replaced all usages of `timestamp_nanos` with `timestamp_nanos_opt` to avoid panics when the timestamp is out of range. | |
| `to_unix_timestamp`, `to_float`, and `uuid_v7` can now return an error if the supplied timestamp is unrepresentable as a nanosecond timestamp. Previously the function calls would panic. |
The changelogs should have the user in mind as the audience. I also think this is a breaking change, no? We'll want to document how to migrate.
There was a problem hiding this comment.
Right, marking this PR as a draft for now. I will try to find some propose a migration plan. I need to refresh my memory on the deprecation process as well.
There was a problem hiding this comment.
We provide linters for such changes. In this way we have eliminated the deprecation of to_timestamp.
I think there is no problem in this case, because the vector during validation will report that to_unix_timestamp returns an error, and this is not handled
closes: #978